Fixed floating point data type check in knowledge vault #1648
Conversation
WalkthroughThe changes in this pull request primarily enhance the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
tests/integration_test/services_test.py (6)
1382-1423: Consider enhancing test coverage with edge cases.The test has good structure with proper setup and cleanup, but could be strengthened by:
- Adding test cases for edge cases (e.g., max/min float values, scientific notation)
- Including descriptive messages in assertions for better debugging
Example improvement:
- assert data_dict['data']['price'] == 54.02 + assert data_dict['data']['price'] == 54.02, f"Expected price to be 54.02 but got {data_dict['data']['price']}"
1459-1459: Remove debug print statement.Debug print statement should be removed before committing.
- print(data_dict)
1424-1468: Consider refactoring common setup code and adding negative number test.The test has duplicate setup code that could be moved to a fixture. Also consider adding test cases for negative integers.
Example fixture:
@pytest.fixture def setup_bot_settings(): bot_settings = BotSettings.objects(bot=pytest.bot).get() bot_settings.content_importer_limit_per_day = 10 bot_settings.cognition_collections_limit = 20 bot_settings.llm_settings['enable_faq'] = True bot_settings.save() yield bot_settings = BotSettings.objects(bot=pytest.bot).get() bot_settings.llm_settings['enable_faq'] = False bot_settings.save()
1505-1505: Remove debug print statements.Multiple debug print statements should be removed.
- print(data_dict)Also applies to: 1529-1529
1510-1511: Remove excessive blank lines.Multiple consecutive blank lines should be removed to maintain consistent spacing.
1469-1538: Consider adding error case tests.While the happy path is well tested, consider adding tests for error cases such as:
- Attempting to update with invalid data types
- Testing boundary conditions
- Testing malformed requests
Example error case:
def test_update_payload_upload_api_with_invalid_float(monkeypatch): # ... setup code ... update_payload = { "data": {"item": "Box", "price": "invalid_float"}, "content_type": "json", "collection": "update_with_float_field_value_integer" } response = client.put( url=f"/api/bot/{pytest.bot}/data/cognition/{pytest.payload_id}", json=update_payload, headers={"Authorization": pytest.token_type + " " + pytest.access_token}, ) actual = response.json() assert actual["error_code"] != 0 assert "invalid float value" in actual["message"].lower()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
kairon/shared/cognition/processor.py(3 hunks)tests/integration_test/services_test.py(1 hunks)
🔇 Additional comments (4)
kairon/shared/cognition/processor.py (3)
271-274: Assigning validated values back to data
The code correctly updates data with validated column values, ensuring data integrity.
295-295: Updating payload with validated data
Assigning the validated data back to payload['data'] ensures that any transformations or validations are reflected when saving cognition data.
322-322:
Critical issue: Unvalidated data being saved
After validating payload['data'], the variable data still holds the unvalidated data. Consequently, payload_obj.data = data assigns unvalidated data to the object. Update data with the validated data to ensure consistency.
Apply this diff to fix the issue:
if content_type == CognitionDataType.json.value:
payload['data'] = CognitionDataProcessor.validate_metadata_and_payload(bot, payload)
+ data = payload['data']
payload_obj.data = dataLikely invalid or redundant comment.
tests/integration_test/services_test.py (1)
1382-1538: Overall good test coverage with room for improvement.
The tests provide good coverage of float field handling in the knowledge vault, including:
- Direct float value storage
- Integer to float conversion
- Update functionality
However, consider the following improvements:
- Move common setup/teardown code to fixtures
- Add error case testing
- Remove debug print statements
- Add edge case testing
Summary by CodeRabbit
New Features
Bug Fixes
Tests